Skip to content

Conversation

@cwasicki
Copy link
Collaborator

@cwasicki cwasicki commented Feb 3, 2025

This add support for cancelled orders which do not have the price or quantity field set. Since the order field is still valid for cancelled orders and price and quantity fields are not designed as optional, we set the fields to NaN.

In addition to that the headers of the CLI tool are fixed.

Cancelled orders don't have a price or quantity, which leads to failures
when converting from protobuf. This allows missing price and quantities.

Signed-off-by: cwasicki <[email protected]>
@cwasicki cwasicki requested a review from a team as a code owner February 3, 2025 19:43
@github-actions github-actions bot added the part:docs Affects the documentation label Feb 3, 2025
Copy link

@nhatcher-frequenz nhatcher-frequenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

or order.price.currency == Currency.UNSPECIFIED
or order.quantity.mw.is_nan()
):
if od.state_detail.state != OrderState.CANCELED:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would flatten these two if statements

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found that too difficult to read, but changed it now.

RELEASE_NOTES.md Outdated

## Bug Fixes

* Add support for cancelled orders that do not have a price or quantity.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would treat this as a bug fix, a more accurate description would be:

* Deal correctly with cancelled orders with no price or quantity

Cancelled orders may not have a price or quantity, all other orders
must have these set.

Signed-off-by: cwasicki <[email protected]>
@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Feb 4, 2025
Signed-off-by: cwasicki <[email protected]>
@cwasicki
Copy link
Collaborator Author

cwasicki commented Feb 4, 2025

Updated and added test

@cwasicki cwasicki enabled auto-merge February 4, 2025 10:18
@cwasicki cwasicki added this pull request to the merge queue Feb 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2025
@cwasicki cwasicki added this pull request to the merge queue Feb 4, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit cace663 Feb 4, 2025
14 checks passed
@cwasicki cwasicki deleted the fix branch February 4, 2025 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants